Conversation
|
|
…l/bin ownership after install Co-authored-by: yihui <163582+yihui@users.noreply.github.com>
/usr/local/bin ownership: drop -R, restore after install
yihui
left a comment
There was a problem hiding this comment.
instead of restoring ownership, let's use a different approach to solve the problem:
we don't tamper with /usr/local/bin at all, but use sudo only once to add TinyTeX's bin path to /etc/paths.d, e.g.
sudo echo path/to/TinyTeX/bin/* > /etc/paths.d/TinyTeX
then we should avoid tlmgr path add for macOS in all places; this command is called in several places in this package, some are through the R function tinytex::tlmgr_path() and some are tlmgr path add commands directly; please find out all of them and avoid them for macOS
the osascript() call in R should only do this /etc/paths.d operation and nothing else; i think we can get rid of check_local_bin() entirely since /usr/local/bin is no longer relevant
Agent-Logs-Url: https://github.com/rstudio/tinytex/sessions/13dd76fe-b745-4b61-8faf-aa3314531d3e Co-authored-by: yihui <163582+yihui@users.noreply.github.com>
Done in eea0101. The approach now:
|
/usr/local/bin ownership: drop -R, restore after install/etc/paths.d/TinyTeX instead of touching /usr/local/bin
yihui
left a comment
There was a problem hiding this comment.
remember to roxygenize after doc changes
R/install.R
Outdated
| #' @param add_path Whether to add the bin path of TeX Live to the system | ||
| #' environment variable \var{PATH}. On macOS, this writes the bin path to | ||
| #' \file{/etc/paths.d/TinyTeX}; on Linux, it runs \command{tlmgr path add}; | ||
| #' on Windows, it runs \command{tlmgr path add}. |
There was a problem hiding this comment.
document the details in tlmgr_path and don't repeat in other places
| #' @param add_path Whether to add the bin path of TeX Live to the system | |
| #' environment variable \var{PATH}. On macOS, this writes the bin path to | |
| #' \file{/etc/paths.d/TinyTeX}; on Linux, it runs \command{tlmgr path add}; | |
| #' on Windows, it runs \command{tlmgr path add}. | |
| #' @param add_path Whether to add the bin path of TeX Live to the system | |
| #' environment variable \var{PATH}. See \code{\link{tlmgr_path}()}. |
| #' \code{use_tinytex()} adds the copy of TinyTeX in an existing folder to the | ||
| #' \code{PATH} variable of the current system (on macOS via | ||
| #' \file{/etc/paths.d/TinyTeX}; on other platforms via \command{tlmgr path add}), | ||
| #' so that you can use utilities such as \command{tlmgr} and \command{pdflatex}, |
There was a problem hiding this comment.
don't mention OS details; link to tlmgr_path documentation
R/tlmgr.R
Outdated
| paths_file = '/etc/paths.d/TinyTeX' | ||
| if (action == 'add') { | ||
| f = getOption('tinytex.tlmgr.path', find_tlmgr(extra = TRUE)) | ||
| if (length(f) == 0 || !file_test('-x', f)) return(invisible(1L)) | ||
| bin = normalizePath(dirname(f)) | ||
| tmp = tempfile(tmpdir = '/tmp') | ||
| writeLines(bin, tmp) | ||
| osascript(sprintf('cp %s %s', tmp, paths_file)) | ||
| unlink(tmp) | ||
| } else { | ||
| osascript(sprintf('rm -f %s', paths_file)) | ||
| } |
There was a problem hiding this comment.
factor out a function macos_path(dir, add = TRUE) so it can be reused both here and in use_tinytex(), where dir is the bin path; this function should check if /etc/paths.d/TinyTeX already exists and contains the desired bin path; if it does, we should skip the creation of this file; the same check should be done in *.sh scripts, too
There was a problem hiding this comment.
Done in 76970e1 — macos_path(dir, add) is factored out and used by both tlmgr_path() and use_tinytex(). It checks grep -qxF in the shell scripts and readLines + identical in R to skip the write when the file already contains the desired path.
R/tlmgr.R
Outdated
| f = getOption('tinytex.tlmgr.path', find_tlmgr(extra = TRUE)) | ||
| if (length(f) == 0 || !file_test('-x', f)) return(invisible(1L)) | ||
| bin = normalizePath(dirname(f)) |
There was a problem hiding this comment.
this is repeating the code in tweak_path(); please always factor out, never copy
There was a problem hiding this comment.
Done in 76970e1 — factored out as find_tinytex_bin() which both tweak_path() and tlmgr_path() (via macos_path()) now use.
tools/install-bin-unix.sh
Outdated
| fi | ||
| # add TinyTeX's bin path to /etc/paths.d instead of creating symlinks in /usr/local/bin | ||
| # (at this point we are in $TEXDIR/bin/*/ after the `cd` above) | ||
| TINYTEX_BINDIR="$(pwd)" |
There was a problem hiding this comment.
this var is not used elsewhere; inline it into printf call next
R/tlmgr.R
Outdated
| } else { | ||
| osascript(sprintf('rm -f %s', paths_file)) | ||
| } | ||
| return(invisible(0L)) |
There was a problem hiding this comment.
return status code of osascript()
There was a problem hiding this comment.
Done in 76970e1 — osascript() now captures and returns the exit status, and macos_path() propagates it via invisible(osascript(...)). tlmgr_path() returns invisible(macos_path(...)) on macOS.
R/tlmgr.R
Outdated
| f = getOption('tinytex.tlmgr.path', find_tlmgr(extra = TRUE)) | ||
| if (length(f) == 0 || !file_test('-x', f)) return(invisible(1L)) | ||
| bin = normalizePath(dirname(f)) | ||
| tmp = tempfile(tmpdir = '/tmp') |
There was a problem hiding this comment.
i don't think there's any reason to specify a hardcoded tmpdir
| tmp = tempfile(tmpdir = '/tmp') | |
| tmp = tempfile() |
…ncy, osascript returns status, roxygenize Agent-Logs-Url: https://github.com/rstudio/tinytex/sessions/3a5f54ec-a104-4514-843a-93ea370973bf Co-authored-by: yihui <163582+yihui@users.noreply.github.com>
Done in 76970e1 — man pages updated to match the doc changes. |
yihui
left a comment
There was a problem hiding this comment.
revert changes in tools/install-unx.sh; it should be rare that any macOS user would run this script; the script is mainly used for building binary TinyTeX in our GHA
in tools/install-bin-unix.sh, is it possible to add support for a --no-path argument? if this flag is given, skip the PATH manipulation
tools/install-bin-unix.sh
Outdated
| # (at this point we are in $TEXDIR/bin/*/ after the `cd` above) | ||
| if ! grep -qxF "$(pwd)" /etc/paths.d/TinyTeX 2>/dev/null; then | ||
| echo "Admin privilege (password) is required to set up the PATH for TinyTeX:" | ||
| printf '%s\n' "$(pwd)" | sudo tee /etc/paths.d/TinyTeX > /dev/null |
There was a problem hiding this comment.
if this fails (e.g. user gave up password input), echo a message telling users how to set up PATH; the command would be something like export PATH=$PATH:$(pwd) (note that in the message,
also run this export command right here, because the /etc/paths.d won't be picked up until the shell is restarted
There was a problem hiding this comment.
Done in 14a151f — on sudo failure, we echo the manual export PATH=$PATH:$(pwd) command with instructions to add it to the shell startup profile. In both success and failure cases, export PATH="$PATH:$(pwd)" is run immediately so the current shell picks it up without a restart.
R/install.R
Outdated
|
|
||
| # add/remove TinyTeX's bin path to/from /etc/paths.d/TinyTeX on macOS; | ||
| # if adding and the file already contains the desired path, skip the operation | ||
| macos_path = function(dir = NULL, add = TRUE) { |
There was a problem hiding this comment.
use the argument action to be consistent with tlmgr_path():
| macos_path = function(dir = NULL, add = TRUE) { | |
| macos_path = function(dir = NULL, action = 'add') { |
R/install.R
Outdated
| "Please run this command in your Terminal (password required):\n sudo ", | ||
| cmd, call. = FALSE | ||
| ) | ||
| invisible(ret) |
There was a problem hiding this comment.
| invisible(ret) | |
| ret |
R/install.R
Outdated
| if (is_macos()) { | ||
| macos_path(normalizePath(d)) | ||
| } else if (system2(p, c('path', 'add')) != 0) stop( |
There was a problem hiding this comment.
for macOS, we also need to stop() when exit status is not 0
R/install.R
Outdated
| if (add) { | ||
| if (is.null(dir) || dir == '') return(invisible(1L)) | ||
| if (file.exists(paths_file) && | ||
| identical(trimws(readLines(paths_file, warn = FALSE)), dir)) | ||
| return(invisible(0L)) | ||
| tmp = tempfile() | ||
| on.exit(unlink(tmp), add = TRUE) | ||
| writeLines(dir, tmp) | ||
| invisible(osascript(sprintf('cp %s %s', tmp, paths_file))) | ||
| } else { | ||
| invisible(osascript(sprintf('rm -f %s', paths_file))) | ||
| } |
There was a problem hiding this comment.
| if (add) { | |
| if (is.null(dir) || dir == '') return(invisible(1L)) | |
| if (file.exists(paths_file) && | |
| identical(trimws(readLines(paths_file, warn = FALSE)), dir)) | |
| return(invisible(0L)) | |
| tmp = tempfile() | |
| on.exit(unlink(tmp), add = TRUE) | |
| writeLines(dir, tmp) | |
| invisible(osascript(sprintf('cp %s %s', tmp, paths_file))) | |
| } else { | |
| invisible(osascript(sprintf('rm -f %s', paths_file))) | |
| } | |
| cmd = if (add) { | |
| if (is.null(dir) || dir == '') return(1L) | |
| if (file.exists(paths_file) && | |
| identical(readLines(paths_file, warn = FALSE), dir)) | |
| return(0L) | |
| tmp = tempfile() | |
| on.exit(unlink(tmp), add = TRUE) | |
| writeLines(dir, tmp) | |
| sprintf('cp %s %s', tmp, paths_file)) | |
| } else { | |
| sprintf('rm -f %s', paths_file) | |
| } | |
| osascript(cmd) |
R/tlmgr.R
Outdated
| if (is_macos()) return(invisible(macos_path(find_tinytex_bin(), action == 'add'))) | ||
| tlmgr(c('path', action), .quiet = TRUE) |
There was a problem hiding this comment.
| if (is_macos()) return(invisible(macos_path(find_tinytex_bin(), action == 'add'))) | |
| tlmgr(c('path', action), .quiet = TRUE) | |
| if (is_macos()) { | |
| invisible(macos_path(find_tinytex_bin(), action == 'add')) | |
| } else tlmgr(c('path', action), .quiet = TRUE) |
…acos_path refactor Agent-Logs-Url: https://github.com/rstudio/tinytex/sessions/4ed6cb3b-42e3-4296-9381-ea23c5541e30 Co-authored-by: yihui <163582+yihui@users.noreply.github.com>
Done in 14a151f — |
tools/install-bin-unix.sh
Outdated
| sudo chown -R `whoami`:admin /usr/local/bin | ||
| # add TinyTeX's bin path to /etc/paths.d instead of creating symlinks in /usr/local/bin | ||
| # (at this point we are in $TEXDIR/bin/*/ after the `cd` above) | ||
| if [ "$1" != '--no-path' ] && ! grep -qxF "$(pwd)" /etc/paths.d/TinyTeX 2>/dev/null; then |
There was a problem hiding this comment.
two issues:
- the --no-path argument is not only for macOS, so raise the condition to the top; when --no-path is present, just skip path manipulation for any OS
- is there a way to way to test the presence of
--no-pathwithout assuming it's $1?
There was a problem hiding this comment.
Done in 67441ce. The --no-path detection now scans all arguments via a for arg in "$@" loop and sets a NO_PATH flag. The entire path manipulation block (both Darwin and Linux branches) is wrapped under if [ $NO_PATH -eq 0 ], so the flag applies to any OS.
R/install.R
Outdated
| if (is_macos()) { | ||
| if (macos_path(normalizePath(d)) != 0) stop( | ||
| "Failed to add '", d, "' to /etc/paths.d/TinyTeX. You may consider the fallback ", | ||
| "approach, i.e., set options(tinytex.tlmgr.path = '", p, "')." | ||
| ) | ||
| } else if (system2(p, c('path', 'add')) != 0) stop( | ||
| "Failed to add '", d, "' to your system's environment variable PATH. You may ", | ||
| "consider the fallback approach, i.e., set options(tinytex.tlmgr.path = '", p, "')." | ||
| ) |
There was a problem hiding this comment.
too much repetition in the two messages; consolidate into one stop() call; it's fine to use the second message for the macOS case, even though the description is not totally accurate
There was a problem hiding this comment.
Done in 67441ce — both branches now assign to ret and share a single stop() call using the second message.
R/install.R
Outdated
| sprintf('cp %s %s', tmp, paths_file) | ||
| } else { | ||
| sprintf('rm -f %s', paths_file) |
There was a problem hiding this comment.
do these paths need to be quoted somehow? what if they contain spaces or literal quotes? would osascript fail in that case?
There was a problem hiding this comment.
Done in 67441ce. Paths are now wrapped with \" (backslash-escaped double quotes) in the shell command string: sprintf('cp \\"%s\\" \\"%s\\"', tmp, paths_file). This produces a command like cp \"/path/with spaces\" \"/etc/paths.d/TinyTeX\", which AppleScript's do shell script correctly interprets as double-quoted shell arguments, handling spaces in paths. Literal double-quotes in paths (extremely unusual for file paths) are not handled, but that edge case is not a practical concern.
R/install.R
Outdated
| identical(readLines(paths_file, warn = FALSE), dir)) | ||
| return(0L) | ||
| tmp = tempfile() | ||
| on.exit(unlink(tmp), add = TRUE) |
There was a problem hiding this comment.
shouldn't unlink tmp on exit because the osascript command may fail, in which case we should keep tmp, otherwise the warning() message in osascript() will point users to a nonexistent file to cp/rm
unlink tmp only if osascript succeeds
There was a problem hiding this comment.
Done in 67441ce. Removed the on.exit(unlink(tmp)) and replaced it with if (add && ret == 0) unlink(tmp) after the osascript() call. The temp file persists on failure, so the sudo cp ... path in the warning message remains valid for the user to run manually.
…k only on success, consolidate stop() Agent-Logs-Url: https://github.com/rstudio/tinytex/sessions/27d65823-31ba-4ff1-ba16-17c9c18bd877 Co-authored-by: yihui <163582+yihui@users.noreply.github.com>
…-filesystem-security-issues
yihui
left a comment
There was a problem hiding this comment.
in R code, the condition to run macos_path() is currently is_macos(); i want to add another condition: test if /usr/local/bin is writable, if it is, follow the old behavior (i.e., run tlmgr path add); if not, use the new macos_path(); remember to update doc for tlmgr_path() accordingly and roxygenize
same thing for .sh: check if /usr/local/bin is writable
existing TinyTeX users may have already chmod /usr/local/bin, and i don't want to bother them with another sudo command to add /etc/paths.d/TinyTeX
| ret = system(sprintf( | ||
| "/usr/bin/osascript -e 'do shell script \"%s\" with administrator privileges'", cmd | ||
| )) != 0) warning( | ||
| )) | ||
| if (ret != 0) warning( | ||
| "Please run this command in your Terminal (password required):\n sudo ", | ||
| cmd, call. = FALSE | ||
| ) |
There was a problem hiding this comment.
before running system() command, emit a message() telling users the command to be executed so they know the purpose of requiring the password; after running system(), if it fails, add warning() to tell you to run the command in terminal (don't repeat the command in the warning message since it has been mentioned in message() before)
There was a problem hiding this comment.
Done in 71d49e3. osascript() now emits message("Requesting admin privilege to run: sudo ", cmd) before the system() call, and on failure emits warning("Please run the above command in your Terminal (password required).") without repeating the command.
R/install.R
Outdated
| sprintf('cp \\"%s\\" \\"%s\\"', tmp, paths_file) | ||
| } else { | ||
| sprintf('rm -f \\"%s\\"', paths_file) |
There was a problem hiding this comment.
don't escape quotes here, but do it in osascript() instead; however, in osascript(), you should keep the original cmd instead of overriding it with the escaped version, because in message() you need to output the original cmd, but for /usr/bin/osascript you need to pass the escaped version
otherwise in message() users would see escaped quotes, and the cmd wouldn't be valid for them to run
There was a problem hiding this comment.
Done in 71d49e3. macos_path() now uses plain sprintf('cp "%s" "%s"', ...) (no escaping). In osascript(), a local escaped = gsub('"', '\\"', cmd, fixed = TRUE) is used for the shell command passed to /usr/bin/osascript, while the original cmd is used in message() so users see clean, runnable commands.
…ipt(); move quote escaping to osascript() Agent-Logs-Url: https://github.com/rstudio/tinytex/sessions/1bca78ef-48fe-4a0f-bf5c-3be15fdc5da8 Co-authored-by: yihui <163582+yihui@users.noreply.github.com>
Done in 71d49e3. Added |
/etc/paths.d/TinyTeX instead of touching /usr/local/bin/etc/paths.d/TinyTeX when /usr/local/bin is not writable
TinyTeX was recursively changing ownership of
/usr/local/bin(chown -R) — affecting all pre-existing binaries unrelated to TinyTeX — and creating symlinks from/usr/local/bininto the user's home directory. This PR eliminates unsafe/usr/local/binmanipulation on macOS, with a graceful fallback for users who have already made/usr/local/binwritable.Changes
R/install.Rcheck_local_bin()entirelymacos_local_bin_writable()helper (file.access('/usr/local/bin', 2) == 0)macos_path(dir, action)helper used by bothtlmgr_path()anduse_tinytex(); skips writing/etc/paths.d/TinyTeXif it already contains the desired path (idempotent)osascript(): emitsmessage()before running to inform users of the command requiring the password; on failure emitswarning()without repeating the command; escapes double-quotes in the shell command internally (keeping the originalcmdfor the message)macos_path(): paths passed to shell commands are quoted to handle paths containing spaces; temp file is only deleted ifosascriptsucceedsuse_tinytex(): usesmacos_path()only whenis_macos() && !macos_local_bin_writable(); warns on failureadd_pathparameter doc simplified to link totlmgr_path()R/tlmgr.Rfind_tinytex_bin()helper to avoid duplicatingtweak_path()logictlmgr_path(): on macOS, usesmacos_path()(writing to/etc/paths.d/TinyTeX) only when/usr/local/binis not writable; if/usr/local/binis writable, falls back to the existingtlmgr path add/removebehavior (preserving existing users' setups); on Linux/Windows the existing behavior is unchangedtools/install-bin-unix.sh/usr/local/binis writable, use./tlmgr path add; otherwise write the current bin directory to/etc/paths.d/TinyTeXviasudo tee(skipped if already up-to-date)--no-pathargument to skip PATH manipulation for any OS (detected by scanning all arguments, not assuming a fixed position)export PATH=$PATH:<bindir>command with instructions to add it to the shell startup profile; also runs the export immediately so the current shell session picks up the path without a restart./tlmgr path addtools/install-unx.shOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.